Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli/buffer): allow Buffer to store MAX_SIZE bytes #6568

Closed
wants to merge 5 commits into from

Conversation

marcosc90
Copy link
Contributor

fixes #6543

@@ -157,31 +157,27 @@ export class Buffer implements Reader, ReaderSync, Writer, WriterSync {

async readFrom(r: Reader): Promise<number> {
let n = 0;
const buf = new Uint8Array(32 * 1024);
Copy link
Contributor Author

@marcosc90 marcosc90 Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calling this.#grow without knowing the actual amount that needs to grow is causing troubles, went with the simpler and less error-prone code, which is calling writeSync which calls this.#grow with the size of the read buffer.

In a few minutes, I'll push an alternative reading directly to the buffer. If someone else wants to try a different approach I'm more than happy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posted alternative implementation in #6570

@marcosc90
Copy link
Contributor Author

marcosc90 commented Jun 29, 2020

In Windows, it seems the ArrayBuffer hard limit is different, so it can't be tested correctly.

https://github.com/denoland/deno/pull/6568/checks?check_run_id=820234333#step:17:931

RangeError: Array buffer allocation failed
    at new ArrayBuffer (<anonymous>)

Or maybe the CI windows instance has less memory:

On my machine:

Ubuntu: throws with 2 ** 35
Windows: throws with 2 ** 34
Deno Buffer MAX SIZE: 2 ** 32 - 2

@marcosc90
Copy link
Contributor Author

Ignored tests in low memory environments:

Windows:

test bufferGrowWriteMaxBuffer ... ignored (0ms)
test bufferGrowReadSyncCloseToMaxBuffer ... ignored (0ms)
test bufferGrowReadCloseToMaxBuffer ... ignored (0ms)
test bufferGrowReadCloseMaxBufferPlus1 ... ignored (0ms)
test bufferGrowReadSyncCloseMaxBufferPlus1 ... ignored (0ms)
test bufferReadCloseToMaxBufferWithInitialGrow ... ignored (0ms)

Linux and Mac

....
test bufferGrowReadCloseMaxBufferPlus1 ... ok (8697ms)
test bufferGrowReadSyncCloseMaxBufferPlus1 ... ok (9522ms)
test bufferReadCloseToMaxBufferWithInitialGrow ... ok (16614ms)
....

@@ -11,11 +11,19 @@ import {
unitTest,
} from "./test_util.ts";

const MAX_SIZE = 2 ** 32 - 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really necessary to allocate 4G of memory? Does the crash occur a smaller allocations?

Copy link
Contributor Author

@marcosc90 marcosc90 Jul 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can replicate the crash on master using write/writeSync when writing ~2gb, but not for readFrom, the latter allocate a maximum buffer of 4294966784, 510 bytes short of MAX_SIZE

const buf = new Deno.Buffer()
buf.writeSync(repeat("x", 2147483647));
buf.writeSync(repeat("x", 1));
// throws The buffer cannot be grown beyond the maximum size.

readFrom/Sync

const reader = new Deno.Buffer(new ArrayBuffer(4294966784 + 1));
const buf = new Deno.Buffer();
buf.readFromSync(reader);
// throws The buffer cannot be grown beyond the maximum size.

Can't replicate with a smaller amount because buf.readForm calls buf.#grow with 512 which always ends up with a max buffer of 4294966784 when trying to read any amount less than 4294966784, so not possible to replicate.

@marcosc90
Copy link
Contributor Author

Closing in favor of #6570

@marcosc90 marcosc90 closed this Jul 9, 2020
@marcosc90 marcosc90 deleted the fix-buffer-grow branch July 10, 2020 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buffer fails to store up to MAX_SIZE bytes
2 participants